Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(react-button): apply hover styles only on devices that support it #31700

Closed
wants to merge 2 commits into from
Closed

fix(react-button): apply hover styles only on devices that support it #31700

wants to merge 2 commits into from

Conversation

mainframev
Copy link
Contributor

@mainframev mainframev commented Jun 13, 2024 •
edited
Loading

We have a "sticky hover" issue on pointer devices due to the lack of differentiation between pointer and hover devices. This problem also exists in v8.

I approached it with the help of media-query any-hover. Hover effect is wrapped into that query, left hover:active outside, so there will be a visual indication after tap on mobile devices.

Browser support

Support is solid

Devices

While the any-hover media query generally helps distinguish between pointer and hover devices, we must be aware of devices with multiple input methods (e.g., pointer devices connected to keyboards and mouse, or laptops with touchscreens). The any-hover: hover query targets any available input method capable of hover, whereas hover: hover applies only to the primary input.

Previous similar issues I found

Previous Behavior

Screen.Recording.2024-06-13.at.13.06.45.mov

New Behavior

Screen.Recording.2024-06-13.at.13.06.10.mov
RPReplay_Final1718277613.MP4

Related Issue(s)

@mainframev mainframev requested a review from a team June 13, 2024 11:15
@fabricteam
Copy link
Collaborator

fabricteam commented Jun 13, 2024 •
edited
Loading

Perf Analysis (@fluentui/react-components)

Scenario Render type Master Ticks PR Ticks Iterations Status
FluentProviderWithTheme virtual-rerender 36 33 10 Possible regression
All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 639 626 5000
Button mount 305 308 5000
Field mount 1130 1159 5000
FluentProvider mount 722 720 5000
FluentProviderWithTheme mount 80 87 10
FluentProviderWithTheme virtual-rerender 36 33 10 Possible regression
FluentProviderWithTheme virtual-rerender-with-unmount 73 69 10
MakeStyles mount 857 869 50000
Persona mount 1782 1718 5000
SpinButton mount 1457 1444 5000
SwatchPicker mount 1620 1636 5000

@fabricteam
Copy link
Collaborator

fabricteam commented Jun 13, 2024 •
edited
Loading

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-breadcrumb
@fluentui/react-breadcrumb - package		 113.657 kB
31.532 kB		 115.873 kB
31.731 kB		 2.216 kB
199 B
react-button
Button		 37.105 kB
10.785 kB		 39.321 kB
10.914 kB		 2.216 kB
129 B
react-button
CompoundButton		 43.516 kB
12.076 kB		 46.084 kB
12.248 kB		 2.568 kB
172 B
react-button
MenuButton		 41.886 kB
12.133 kB		 44.149 kB
12.314 kB		 2.263 kB
181 B
react-button
SplitButton		 49.898 kB
13.725 kB		 52.409 kB
13.922 kB		 2.511 kB
197 B
react-button
ToggleButton		 53.032 kB
12.545 kB		 56.711 kB
12.733 kB		 3.679 kB
188 B
react-components
react-components: Button, FluentProvider & webLightTheme		 69.141 kB
20.157 kB		 71.357 kB
20.336 kB		 2.216 kB
179 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover		 211.741 kB
60.957 kB		 213.957 kB
61.16 kB		 2.216 kB
203 B
react-components
react-components: entire library		 1.093 MB
270.372 kB		 1.097 MB
270.801 kB		 4.326 kB
429 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-card
Card - All		 100.845 kB
28.873 kB
react-card
Card		 93.629 kB
27.077 kB
react-card
CardFooter		 14.356 kB
5.798 kB
react-card
CardHeader		 16.879 kB
6.664 kB
react-card
CardPreview		 14.42 kB
5.934 kB
react-components
react-components: FluentProvider & webLightTheme		 44.442 kB
14.607 kB
react-dialog
Dialog (including children components)		 98.908 kB
29.807 kB
react-message-bar
MessageBar (all components)		 24.413 kB
9.117 kB
react-portal-compat
PortalCompatProvider		 8.39 kB
2.64 kB
react-tag-picker
@fluentui/react-tag-picker - package		 180.139 kB
54.231 kB
react-timepicker-compat
TimePicker		 103.427 kB
34.598 kB
🤖 This report was generated against b059abe27750129616c2324b9e567ee5772dcb7c

fabricteam
fabricteam reviewed Jun 13, 2024
View reviewed changes
change/@fluentui-react-button-aaf5c99d-c063-4562-9ec1-42c216a4b2d7.json Outdated
@@ -0,0 +1,7 @@
{
Copy link
Collaborator

@fabricteam fabricteam Jun 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕵 fluentuiv9 Open the Visual Regressions report to inspect the affected screenshots

Card Converged 2 screenshots
Image Name Diff(in Pixels) Image Type
Card Converged.appearance - High Contrast.normal.chromium.png 0 Removed
Card Converged.appearance.normal.chromium.png 0 Removed
Card Converged - Interactive 9 screenshots
Image Name Diff(in Pixels) Image Type
Card Converged - Interactive.appearance interactive - Filled - High Contrast.click.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled - High Contrast.hover.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled - High Contrast.normal.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled Alternative - High Contrast.click.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled Alternative - High Contrast.hover.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Filled Alternative - High Contrast.normal.chromium.png 0 Added
Card Converged - Interactive.appearance interactive - Outline - Dark Mode.click.chromium.png 0 Removed
Card Converged - Interactive.appearance interactive - Outline - Dark Mode.hover.chromium.png 0 Removed
Card Converged - Interactive.appearance interactive - Outline - Dark Mode.normal.chromium.png 0 Removed
Card Converged - Selectable 8 screenshots
Image Name Diff(in Pixels) Image Type
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.click.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.hover.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.normal.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - Dark Mode.selected.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.click.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.hover.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.normal.chromium.png 0 Removed
Card Converged - Selectable.appearance selectable - Subtle - High Contrast.selected.chromium.png 0 Removed
SwatchPicker Converged 1 screenshots
Image Name Diff(in Pixels) Image Type
SwatchPicker Converged.Shape.default.chromium.png 0 Added

@mainframev mainframev marked this pull request as ready for review June 13, 2024 12:07
@mainframev mainframev requested review from khmakoto and a team as code owners June 13, 2024 12:07
@mainframev mainframev force-pushed the fix/button-hover-mobile branch from 5f0ae01 to b5037aa Compare June 13, 2024 12:52
layershifter
layershifter requested changes Jun 17, 2024
View reviewed changes
Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have a discussion about this on Tech sync first.

For example, I have following concerns:

  • this PR adds a bundle size hit
  • consumers' overrides for :hover selector will stop to work as a new selector will be @media (any-hover: hover)

@spmonahan
Copy link
Contributor

Let's have a discussion about this on Tech sync first.

For example, I have following concerns:

  • this PR adds a bundle size hit
  • consumers' overrides for :hover selector will stop to work as a new selector will be @media (any-hover: hover)

+1 this. While I really like these newer selectors that, in theory, allow us to better support multiple input modes my past experience has been that they are not uniformly implemented and need lots of testing. As part of further discussion I'd propose we have a device support/testing matrix for features like this.

@mainframev
Copy link
Contributor Author

@layershifter @spmonahan thank you guys! As suggested, let's discuss more in detail on tech sync 👍🏻

@mainframev mainframev force-pushed the fix/button-hover-mobile branch from b5037aa to 6ad0204 Compare June 18, 2024 08:45
@mainframev mainframev requested a review from fabricteam June 18, 2024 09:14
@mainframev mainframev mentioned this pull request Jun 28, 2024
Open
1 task
@mainframev
Copy link
Contributor Author

Closing in favor of feature request #31858.

@mainframev mainframev closed this Jun 28, 2024
@mainframev mainframev self-assigned this Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@layershifter layershifter layershifter requested changes

@khmakoto khmakoto Awaiting requested review from khmakoto khmakoto is a code owner

@fabricteam fabricteam Awaiting requested review from fabricteam

@dmytrokirpa dmytrokirpa Awaiting requested review from dmytrokirpa

@ValentinaKozlova ValentinaKozlova Awaiting requested review from ValentinaKozlova

Assignees

@mainframev mainframev

Labels
Area: Accessibility Component: Button Needs: Attention
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Sticky hover effect on mobile
4 participants
@mainframev @fabricteam @spmonahan @layershifter